fix(evmrpc): stream request-body budget charging to close slowloris gap (PLT-780) - #3836
fix(evmrpc): stream request-body budget charging to close slowloris gap (PLT-780)#3836amir-deris wants to merge 1 commit into
Conversation
…ap (PLT-780) Charge the global request-byte budget incrementally as bytes are read instead of reserving Content-Length up front, so a stalled/slow body can no longer pin the shared budget while barely sending data. Adds a per-chunk body read idle timeout (408) as a backstop independent of the overall ReadTimeout, and moves JWT ahead of the byte limiter so unauthenticated clients can't touch the budget at all. Also fixes two issues found while validating the fix: seiLegacyHTTPGate (always present in production) was writing its own response on a failed body read, masking the limiter's 429/408 behind a 400 and leaking internal error text; and the gate closing the body right after buffering it was releasing the budget before the request finished processing instead of holding it for the whole request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3836 +/- ##
==========================================
- Coverage 61.28% 60.41% -0.88%
==========================================
Files 2351 2259 -92
Lines 197283 186877 -10406
==========================================
- Hits 120907 112894 -8013
+ Misses 65530 63987 -1543
+ Partials 10846 9996 -850
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Adds JWT authentication is moved ahead of Reviewed by Cursor Bugbot for commit c77c1b3. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c77c1b3. Configure here.
| b.release() | ||
| } | ||
| return n, err | ||
| } |
There was a problem hiding this comment.
Body idle deadline never cleared
High Severity
budgetBody.Read sets a connection SetReadDeadline before every chunk, but neither the EOF path nor Close resets it to zero. With the default 10s body_read_idle_timeout, that deadline stays armed through the rest of the request. Handlers that run longer than 10s after the body is consumed can have their request context cancelled (and keep-alive reuse disrupted), cutting off legitimate slow RPCs such as traces, estimates, and large log queries.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c77c1b3. Configure here.
| // cw suppresses the inner handler's own response once outcome is set, so the | ||
| // status/message below always wins over whatever the inner handler wrote. | ||
| cw := &captureResponseWriter{ResponseWriter: w, outcome: &outcome} | ||
| l.inner.ServeHTTP(cw, r) | ||
|
|
||
| if budgetWrapped != nil { | ||
| // Release only after the inner handler chain fully returns, so the budget | ||
| // stays held for the whole request even if an inner handler (e.g. the | ||
| // sei-legacy gate) closes the body early after buffering it. | ||
| _ = budgetWrapped.Close() | ||
| budgetWrapped.release() | ||
| } |
There was a problem hiding this comment.
🔴 requestSizeLimiter.ServeHTTP now releases the global byte budget (budgetWrapped.Close()/release()) in plain sequential code after l.inner.ServeHTTP(cw, r) returns, instead of the old defer l.budget.Release(weight). A panic that escapes the inner handler chain (e.g. seiLegacyHTTPGate's JSON batch-merge logic, or the gzip/vhost/cors wrappers, none of which are covered by go-ethereum's per-method-callback recover) skips both cleanup calls, permanently leaking the reserved bytes from the shared max_concurrent_request_bytes semaphore. Repeated panics exhaust the budget and cause legitimate requests to be rejected with 429 until process restart. Fix: wrap the Close()/release() calls in a defer.
Extended reasoning...
The bug. Before this PR, requestSizeLimiter.ServeHTTP released the global byte-budget semaphore with defer l.budget.Release(weight), so the reservation was returned even if the inner handler panicked. This PR replaces incremental up-front reservation with per-chunk charging via budgetBody, and moves the release to plain sequential code in request_limiter.go:88-99:
l.inner.ServeHTTP(cw, r)
if budgetWrapped != nil {
_ = budgetWrapped.Close()
budgetWrapped.release()
}There is no defer and no recover around these two calls. If l.inner.ServeHTTP panics, both Close() and release() are skipped entirely, and the panic unwinds straight past this frame.
Why the panic can actually reach here. The pinned go-ethereum fork's RPC server only recovers panics inside the per-method callback (rpc/service.go, callback.call) — that is, only once a JSON-RPC method handler is actually invoked. Everything upstream of that in the chain this PR wires — seiLegacyHTTPGate (which does its own io.ReadAll plus JSON batch-parsing/merging with slice/map indexing such as entries[idx]/used[idx], with no recover of its own), the gzip/vhost/cors middleware, and rpc.Server's own HTTP dispatch/decode path, are not covered by that recover. A panic anywhere in that surface propagates directly through requestSizeLimiter.ServeHTTP, which also has no recover, up to net/http's per-connection recover — the process survives, but the connection-level recover has no knowledge of the semaphore, so the reservation is never returned.
Why the leak is material, not just theoretical. seiLegacyHTTPGate buffers the whole body via io.ReadAll before it starts its batch-merge logic. budgetBody.Read flushes all unbilled bytes into the semaphore (b.reserved) on EOF, so by the time the gate's merge logic (or anything after the body read) runs, the full body's worth of bytes — up to maxBody (5 MiB by default) — is already charged to the process-global max_concurrent_request_bytes semaphore (128 MiB by default). A single panic there leaks up to 5 MiB; roughly 25 such panics would exhaust the entire budget, after which every legitimate request gets rejected with HTTP 429 until the process restarts. This is a persistent, resource-exhaustion DoS — notably the exact failure class this PR's own description says it is closing (the slowloris/budget-exhaustion gap).
Step-by-step proof:
- Client sends a JSON-RPC batch request containing a gated
sei_*method plus a malformed entry designed to trigger a panic inseiLegacyHTTPGate's merge logic (e.g. an out-of-range index reachable through itsentries[idx]/used[idx]bookkeeping) — or any other panic reachable in the gzip/vhost/cors/dispatch layers. requestSizeLimiter.ServeHTTPwrapsr.BodyinbudgetBody, which charges bytes tol.budget(the shared semaphore) as the gate reads the body viaio.ReadAll, and flushes the remainder intob.reservedon EOF — up to the full body size.l.inner.ServeHTTP(cw, r)is called, which invokes the gate; the gate's merge logic panics.- The panic unwinds through
requestSizeLimiter.ServeHTTPwithout executingbudgetWrapped.Close()/release(), because there is nodefer. - The panic is finally caught by
net/http's connection-level recover; the connection is dropped, butl.budget'sWeightedsemaphore still shows the bytes from step 2 as acquired — permanently, for the life of the process. - Repeating steps 1-5 ~25 times (5 MiB each against a 128 MiB budget) exhausts
max_concurrent_request_bytesentirely; all subsequent legitimate JSON-RPC requests are rejected with HTTP 429 until the node operator restarts the process.
Why nothing else prevents this. budgetBody.fail() handles the ordinary error paths (budget exhaustion mid-read, idle timeout) by calling b.release() itself, but that only covers errors returned from Read() — not a panic thrown by code that has already consumed the body downstream. The per-method recover() wrappers used elsewhere in evmrpc/*.go (e.g. in tracers.go, send.go, block.go) only protect the individual RPC method being called; they don't run for panics in the HTTP-layer plumbing this PR touches.
The fix is exactly what the old code already did: wrap the cleanup in a defer so the budget is released unconditionally, regardless of whether l.inner.ServeHTTP returns normally or panics, e.g. defer func() { _ = budgetWrapped.Close(); budgetWrapped.release() }() placed immediately after budgetWrapped is constructed.
| func (b *budgetBody) Read(p []byte) (int, error) { | ||
| if b.idleTimeout > 0 && b.rc != nil { | ||
| if err := b.rc.SetReadDeadline(time.Now().Add(b.idleTimeout)); err != nil { | ||
| // ResponseController may be unavailable on exotic ResponseWriters; proceed | ||
| // without the idle guard rather than failing the request. | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 The SetReadDeadline error branch in budgetBody.Read (evmrpc/request_limiter.go:127-131) is an empty if block containing only a comment, which trips staticcheck's SA9003 check and fails make lint / the golangci CI workflow as committed. Fix by giving the branch an actual no-op statement (e.g. logging the error) instead of just a comment.
Extended reasoning...
What the bug is. In budgetBody.Read (evmrpc/request_limiter.go), the idle-deadline guard is:
if b.idleTimeout > 0 && b.rc != nil {
if err := b.rc.SetReadDeadline(time.Now().Add(b.idleTimeout)); err != nil {
// ResponseController may be unavailable on exotic ResponseWriters; proceed
// without the idle guard rather than failing the request.
}
}The inner if err != nil { ... } branch contains nothing but a comment. staticcheck's SA9003 rule treats a comment as a non-statement, so from the analyzer's point of view this branch is empty — a classic "did you forget to handle this error" pattern, and the check fires regardless of the intent conveyed in the comment.
How it manifests / proof. I reproduced this directly against the repo's own toolchain:
$ golangci-lint --version
golangci-lint has version 2.5.0 built with go1.25.1 ...
$ golangci-lint run --enable-only=staticcheck ./evmrpc/
evmrpc/request_limiter.go:128:3: SA9003: empty branch (staticcheck)
if err := b.rc.SetReadDeadline(time.Now().Add(b.idleTimeout)); err != nil {
^
1 issues:
* staticcheck: 1
This is the exact line the PR adds as part of the new budgetBody type backing the streamed body-budget charging feature.
Why nothing else in the repo prevents it. .golangci.yml sets default: none and then explicitly re-enables staticcheck in linters.enable, with no per-check staticcheck.checks override that would disable SA9003, and no path-based exclusion covering request_limiter.go (only generated *.pb.go files and one gosec weak-RNG rule are excluded). request_limiter.go is ordinary non-test source, so run.tests: false in the config doesn't exempt it. AGENTS.md/CLAUDE.md state that make lint (which runs golangci-lint per .golangci.yml) must be clean, and .github/workflows/golangci.yml runs the same golangci-lint version in CI. So this is a genuine, reproducible CI-blocking failure introduced by this PR, not a style nit that a reviewer could wave through.
Impact. As committed, make lint fails locally and the golangci-lint GitHub Actions check fails on the PR, blocking merge until fixed. It's purely a lint/CI-gate issue — there's no runtime behavior bug here (the surrounding comment's stated intent, "proceed without the idle guard rather than failing the request," is a reasonable behavior and doesn't need to change).
Fix. Give the branch a real no-op statement instead of only a comment, e.g.:
if err := b.rc.SetReadDeadline(time.Now().Add(b.idleTimeout)); err != nil {
logger.Debug("SetReadDeadline unavailable for body read idle timeout", "err", err)
}or equivalent (_ = err plus a comment, or restructuring to drop the nested if entirely). Either resolves SA9003 while preserving the existing fallback behavior.
| } | ||
|
|
||
| func (b *budgetBody) Read(p []byte) (int, error) { | ||
| if b.idleTimeout > 0 && b.rc != nil { | ||
| if err := b.rc.SetReadDeadline(time.Now().Add(b.idleTimeout)); err != nil { | ||
| // ResponseController may be unavailable on exotic ResponseWriters; proceed | ||
| // without the idle guard rather than failing the request. | ||
| } | ||
| } | ||
|
|
||
| n, err := b.inner.Read(p) | ||
| if n > 0 && b.budget != nil { |
There was a problem hiding this comment.
🔴 budgetBody.Read resets the socket's read deadline to now+body_read_idle_timeout (default 10s) on every chunk via http.ResponseController.SetReadDeadline, which writes directly to the same underlying connection deadline that net/http arms once as the absolute requestStart+ReadTimeout (default 30s) bound for the whole request body read.
Extended reasoning...
budgetBody.Read (evmrpc/request_limiter.go:126-133) calls rc.SetReadDeadline(time.Now().Add(idleTimeout)) unconditionally on every single Read invocation whenever bodyReadIdleTimeout > 0 — which it is by default (BodyReadIdleTimeout: 10 * time.Second in config.go). http.ResponseController.SetReadDeadline is not a separate, layered "idle" deadline; per the Go 1.25.6 net/http source (server.go:499-500), (*response).SetReadDeadline calls c.conn.rwc.SetReadDeadline directly — the exact same socket-level deadline that net/http's readRequest arms once, after headers are parsed, to wholeReqDeadline = t0.Add(Server.ReadTimeout) (server.go:1018-1086). That absolute deadline exists specifically to bound "the maximum duration for reading the entire request, including the body" (the documented semantics of ReadTimeout, echoed in this repo's own evmrpc/config/config.go doc-comment).
Because every chunk read re-arms the deadline to now + idleTimeout instead of computing min(now+idleTimeout, wholeReqDeadline), a client that sends any nonzero amount of body data at an interval shorter than body_read_idle_timeout (e.g. 1 byte every 9 seconds, since the default idle timeout is 10s) keeps pushing the deadline forward indefinitely. Neither guard ever fires: the idle timeout never sees a >10s gap, and the absolute 30s ReadTimeout it silently overwrote can never trip because it no longer exists on the socket. The read is then bounded only by MaxBytesReader's total byte cap (5 MiB by default), which at a 1-byte/9s trickle is on the order of hundreds of days of wall-clock time — effectively unbounded. For that entire duration the connection, its serving goroutine, and one of the max_open_connections slots (default 2000) are pinned.
This is a straightforward regression in the exact direction opposite to what the PR sets out to fix: PLT-780 closed a slowloris gap where a stalled body could pin a large byte-budget reservation cheaply. This change reintroduces a connection/goroutine-lifetime slowloris vector — one that the pre-existing ReadTimeout used to close at a hard 30s ceiling for the body-read phase, and which is now open again, active by default, even in configurations where the byte budget itself is disabled (the wrapping only requires bodyReadIdleTimeout > 0, independent of budget != nil). There is no other mitigation in the stack: per-IP rate limiting isn't wired into this HTTP middleware chain, and even if it were, a rate limiter bounds request arrival rate, not the lifetime of a single already-admitted connection.
The PR's own new tests do not catch this because they build their servers with httptest.NewServer, which never sets http.Server.ReadTimeout. With no absolute deadline present in the test harness, there is nothing for the per-chunk reset to clobber, so TestRequestSizeLimiter_bodyReadIdleTimeout and TestRequestSizeLimiter_steadySlowUploadSucceeds both pass despite the regression. In production, rpcstack.go's Start() always sets http.Server{ReadTimeout: h.timeouts.ReadTimeout}, and CheckTimeouts (evmrpc/endpoints.go) guarantees that value is never below 1s, sanitizing it up to the 30s go-ethereum default when unset — so the absolute deadline is always present in the real server.
Proof walkthrough:
- Server starts with
ReadTimeout = 30s(default) andBodyReadIdleTimeout = 10s(default). - A malicious client opens a POST with
Content-Length: 5000000(near the 5 MiB cap) and sends only 1 byte, then goes idle for 9 seconds, then sends 1 more byte, repeating indefinitely. - At
t=0, after headers are read,net/httparmswholeReqDeadline = t0 + 30son the socket. - At
t≈0,requestSizeLimiterwraps the body inbudgetBody; the inner handler callsRead, which first callsrc.SetReadDeadline(now + 10s)— overwriting the socket deadline fromt0+30sdown tot0+10s(this alone already shortens the "whole request" cap, but at least still bounds it). - At
t≈9s, the 1-byte chunk arrives,Readreturns 1 byte, and the nextReadcall immediately resets the deadline tot≈9s + 10s = t≈19s. - This repeats every ~9s forever:
t=9s → deadline 19s,t=18s → deadline 28s,t=27s → deadline 37s(already past the original 30s absolute cutoff, which no longer exists), and so on without bound. - The connection survives past the documented 30s
ReadTimeoutand continues indefinitely, limited only by the ~5 MiBMaxBytesReadercap — at 1 byte/9s that is roughly 5×10^6 × 9s ≈ 1.4 years of wall-clock time to exhaust, i.e., practically unbounded for operational purposes.
Fix: compute the idle deadline as min(now+idleTimeout, wholeReqDeadline) rather than unconditionally overwriting the socket deadline forward. Since wholeReqDeadline isn't directly exposed via ResponseController, an equivalent fix is to track an absolute deadline computed once when the wrapped body is constructed (readStart + ReadTimeout, threaded in from config) and cap each per-chunk SetReadDeadline call at min(now+idleTimeout, absoluteDeadline).


Summary
Fixes PLT-780: the EVM HTTP
requestSizeLimiterreserved global byte-budget capacity based on the declaredContent-Lengthbefore the body was read, and held that reservation across the full request. A slow/stalled body (slowloris-style) could pin a large reservation cheaply, exhausting the shared budget and denying the endpoint — including for JWT-protected deployments, since the limiter sat outside the JWT check.Content-Length/reservingmaxBodyup front. A stalled body now pins at most one batch, not its declared size.body_read_idle_timeout(default 10s): a per-chunk idle guard viahttp.ResponseController.SetReadDeadline, independent of the overallReadTimeout, so a stalled read is cut and its reservation released quickly (HTTP 408).requestSizeLimiter— unauthenticated clients get 401 without ever touching the shared budget.budget_midread/slow_bodyonrequestRejectedCountfor the new rejection paths.Fixes found while validating
seiLegacyHTTPGate— always present in the production stack — buffers the whole body via its ownio.ReadAlland, on a failed read, was writing its own400response with the raw internal error text before the limiter's outcome-based429/408could apply.captureResponseWriternow suppresses inner-handler writes once the limiter's outcome is set, so the documented status/message always reaches the client.Body.Close(), this freed the reservation the instant the body finished buffering rather than holding it for the whole request — dropping protection against concurrent slow processing of already-read bodies. Release is now decoupled fromClose()and happens once, after the inner handler chain fully returns.Test plan
go test ./evmrpc/...andgo test ./evmrpc/config/...go test ./evmrpc/ -raceon the request-limiter/legacy-gate testsseiLegacyHTTPGategofmt -s -l/goimports -lclean on all touched files